Skip to content

fix(findrive): enforce 255 char limit on filename to prevent DoS (fix…#384

Open
deveshreddyp wants to merge 3 commits into
GenAI-Security-Project:mainfrom
deveshreddyp:fix/issue-355-filename-length
Open

fix(findrive): enforce 255 char limit on filename to prevent DoS (fix…#384
deveshreddyp wants to merge 3 commits into
GenAI-Security-Project:mainfrom
deveshreddyp:fix/issue-355-filename-length

Conversation

@deveshreddyp
Copy link
Copy Markdown

@deveshreddyp deveshreddyp commented Mar 27, 2026

Resolves #355

Summary of Changes

This PR addresses Bug_146 by implementing a strict 255-character length limit on the filename parameter in the upload_file tool to prevent database bloat and potential DoS vectors.

  • Fail-Fast Optimization: I specifically placed the $O(1)$ string length check before the existing $O(N)$ UTF-8 file content encoding check. This ensures the system rejects maliciously long filenames immediately without wasting CPU cycles processing the payload.
  • Proactive Test Coverage: The tests/unit/mcp/test_findrive.py file wasn't on main yet, so I went ahead and created the test suite via TDD to fulfill the exact Acceptance Criteria.

Bugs Resolved

Test Plan

  • test_fd_upload_001_upload_returns_file_id_and_metadata -> Passes (Valid actions still process normally)
  • test_fd_str_002_very_long_filename_accepted_without_validation -> Passes (Correctly returns the error dictionary for oversized filenames)

cc @steadhac @saikishu

saikishu
saikishu approved these changes Mar 27, 2026
View reviewed changes
Comment thread finbot/mcp/servers/findrive/server.py Outdated
"folder": f.folder_path,
"status": "uploaded",
}
return upload_file_action(
Copy link
Copy Markdown
Collaborator

@saikishu saikishu Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not add the defense without refactoring into another function?
keep it simple until there is need for reusability

saikishu
saikishu requested changes Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@saikishu saikishu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove indirection

@deveshreddyp
Copy link
Copy Markdown
Author

deveshreddyp commented Mar 27, 2026

Good call! I initially extracted it to make importing it into the new pytest suite a bit cleaner without tripping over the @mcp.tool decorator context, but you are totally right—it adds unnecessary indirection.

I'm reverting the refactor now and putting the 255-char defense back directly inline. Will push the update in a few minutes!

@deveshreddyp
Copy link
Copy Markdown
Author

deveshreddyp commented Mar 27, 2026

Done! I removed the upload_file_action indirection and moved the 255-char validation back into the main @mcp.tool function. The pytest suite has been updated to unwrap the tool natively, and all tests are passing green. Let me know if anything else is needed!

@steadhac
Copy link
Copy Markdown
Contributor

steadhac commented Mar 28, 2026

@deveshreddyp, here is the test suite PR #317 — FinDrive Server Tests
Add a comprehensive unit test suite for the FinDrive MCP server — the mock file storage
system used by agents to upload, retrieve, list, delete, and search files. Tests cover
all 5 tools, namespace isolation, vendor session access boundaries, filename security, file type validation, upload limits, and edge cases.

The bug was identified with this test suite, which is ready for review; there is no need to create extra tests.
@saikishu Please merge the tests first.

@deveshreddyp
Copy link
Copy Markdown
Author

deveshreddyp commented Mar 29, 2026

Ah, that makes perfect sense! I didn't spot PR #317 pending in the queue, so I proactively wrote a suite locally to test the fix via TDD.

Your test suite looks incredibly comprehensive. I have just removed my custom test_findrive.py file from this PR so it doesn't step on your toes or cause any merge conflicts.

Once PR #317 is merged, this $O(1)$ validation fix is ready to slot right in and should pass your test_fd_str_002_very_long_filename test case perfectly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saikishu saikishu saikishu requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug_146_MUST_FIX: FD-STR-002 — 500-character filename accepted; no length validation

3 participants

@deveshreddyp @steadhac @saikishu